Skip to content

feat(platform): make paths, manifests, and Unix workspaces platform-aware - #328

Merged
bobtista merged 14 commits into
developmentfrom
feat/cross-platform-fixes
Jul 30, 2026
Merged

feat(platform): make paths, manifests, and Unix workspaces platform-aware#328
bobtista merged 14 commits into
developmentfrom
feat/cross-platform-fixes

Conversation

@bobtista

@bobtista bobtista commented Jul 29, 2026

Copy link
Copy Markdown

Summary

Make shared GenHub runtime behavior platform-aware across Windows and Unix. This fixes platform-specific settings and data paths, corrects Unix workspace materialization, and introduces deterministic manifest variants needed by native clients.

This PR contains no GenHub.MacOS project changes and can be reviewed independently of the macOS host.

Changes

  • Resolve Options.ini and application-data consumers through the configured platform paths.
  • Centralize executable classification and add deterministic platform-variant and entry-point resolution.
  • Preserve compatibility with existing flat manifests while rejecting unresolved variant usage.
  • Implement Unix hard links with link(2) and make Unix symlink strategies reachable.
  • Set execute permissions on workspace-owned copies rather than shared CAS blobs.
  • Add behavioral coverage for path conventions, manifest variants, executable classification, and Unix workspace behavior.

Testing

  • dotnet test GenHub/GenHub.Tests/GenHub.Tests.Core/GenHub.Tests.Core.csproj -c Release --no-restore
  • 1,391 passed, 0 failed.

Risks and rollback

Unix workspace behavior changes: hard-link strategies now create real hard links instead of silently copying. Executable files are detached before permission changes so shared CAS blobs are not mutated.

Existing flat manifests remain supported. No persisted-data migration is required, and the changes are revert-safe.

Related issues

Fixes #314
Fixes #316
Part of #327

Greptile Summary

The PR centralizes platform-aware manifest and workspace behavior.

  • Adds manifest variants, deterministic entry-point resolution, and ingestion gating.
  • Adds native Unix hard-link handling and symlink capability registration.
  • Moves Unix execute-bit changes onto intended workspace-owned copies.
  • Routes application-data and game-settings paths through platform providers.

Confidence Score: 2/5

This PR is not yet safe to merge because most Unix strategies skip executable permission isolation, hard-link permission failures can bypass copy fallback, and writable workspaces can mutate shared CAS content.

The execute-permission fix is only reached by SymlinkOnlyStrategy, EACCES escapes the new hard-link fallback, and intentional hard links leave non-executable CAS objects writable through workspace paths, allowing cross-profile content corruption.

Files Needing Attention: GenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.cs, GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs, and the FullCopy/HybridCopySymlink/HardLink strategy implementations

Security Review

HardLinkStrategy exposes writable CAS inodes to launched workspace processes, allowing workspace writes to alter shared content subsequently consumed by other profiles.

Important Files Changed

Filename Overview
GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs Adds native Unix hard links and copy fallback, but EACCES bypasses fallback and writable hard links expose shared CAS bytes to workspace mutation.
GenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.cs Adds copy-on-write executable permission handling, but the call is placed in a dispatcher bypassed by FullCopy, HybridCopySymlink, and HardLink.
GenHub/GenHub.Core/Models/Manifest/ManifestVariantResolver.cs Adds deterministic platform and entry-point resolution while returning the matched manifest file's canonical path spelling.
GenHub/GenHub.Core/Models/Manifest/ManifestIngestionGate.cs Rejects unsupported variant manifests before existing flat-file consumers can mishandle them.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    Manifest[Resolved manifest] --> Strategy[Workspace strategy]
    Strategy -->|FullCopy| Copy[Independent files]
    Strategy -->|Symlink| Symlink[CAS or installation symlinks]
    Strategy -->|HardLink| HardLink[Shared CAS inodes]
    Copy --> Launch[Launch workspace executable]
    Symlink --> Launch
    HardLink --> Launch
    Launch --> Mutation[Game or plugin writes workspace file]
    Mutation -->|same inode| CAS[Shared CAS blob changes]
    CAS --> Other[Other profiles materialize modified bytes]
Loading
Prompt To Fix All With AI
### Issue 1
GenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.cs:502-504
**Executable isolation skips strategies**

When a Unix profile uses `FullCopy`, `HybridCopySymlink`, or `HardLink`, those strategies materialize files through their own loops and bypass `ProcessManifestFileAsync`, so `EnsureExecutableAsync` never runs. Executables retain a non-executable mode in copy-based workspaces, while hard-linked executables are not detached from CAS as intended, causing launch failures or shared-inode permission changes.

### Issue 2
GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs:109
**EACCES bypasses copy fallback**

When `link(2)` returns `EACCES` while `HardLinkStrategy` materializes a CAS file, `CreateHardLinkAsync` throws `UnauthorizedAccessException`, which is not caught by this `IOException` handler. The per-file copy fallback is skipped and workspace preparation aborts instead of producing an independent copy.

### Issue 3
GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs:107-108
**Workspace writes corrupt shared CAS**

When a launched game or plugin writes to a non-executable content-addressable file in a hard-link workspace, the write changes the same user-writable inode stored under the CAS hash. Another profile can then materialize the modified bytes and receive corrupted or incorrect content. **How this was verified:** `HardLinkStrategy` requests real CAS hard links, while copy-on-write isolation applies only to files marked executable and ordinary materialization does not universally revalidate the blob hash.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (4): Last reviewed commit: "fix(launching): attribute the symlink do..." | Re-trigger Greptile

Greptile also left 3 inline comments on this PR.

Context used:

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Added support for platform-specific manifest variants and runtime-aware file and entry-point resolution.
    • Improved executable detection across native binaries, scripts, libraries, and data files.
    • Added platform-specific game settings paths for Linux and macOS.
    • Improved workspace handling with capability-aware symlink selection and Unix hard-link support.
  • Bug Fixes

    • Prevented unsupported manifest variants from being silently processed.
    • Preserved stored content when applying executable permissions.
    • Improved workspace validation and launch-entry-point error reporting.

Walkthrough

The PR adds artifact-variant manifest contracts and fail-closed ingestion, centralizes executable classification, introduces platform-specific game paths and symlink capability detection, and adds Unix hard-link plus atomic executable materialization support with corresponding tests.

Changes

Manifest and platform workspace changes

Layer / File(s) Summary
Manifest variant contracts, resolution, and ingestion gating
GenHub/GenHub.Core/Models/Manifest/*, GenHub/GenHub.Features/Manifest/*, GenHub/GenHub.Tests/.../Manifest/*
Adds variant models, runtime/file/entry-point resolution, format-version rejection, and ingestion checks across discovery, caching, and provider paths.
Executable classification and manifest integration
GenHub/GenHub.Core/Utilities/*, GenHub/GenHub/Features/Content/..., GenHub/GenHub/Features/Manifest/..., GenHub/GenHub/Features/GameProfiles/...
Replaces extension-specific executable checks with shared permission and legacy-launch classification.
Platform paths, symlink capabilities, and service wiring
GenHub/GenHub/Features/GameSettings/*, GenHub/GenHub/Features/GameProfiles/*, GenHub/GenHub.Windows/*, GenHub/GenHub.Linux/*, GenHub/GenHub/Infrastructure/*
Adds platform-specific settings paths and symlink capability providers, updates workspace strategy selection, and uses configured application-data paths in services.
Unix linking and atomic executable materialization
GenHub/GenHub/Features/Workspace/*, GenHub/GenHub.Tests/.../Workspace/*
Adds Unix hard-link and native permission operations, rejects unsupported base hard-link calls, and materializes executable files through temporary copies and atomic replacement.

Estimated code review effort: 5 (Critical) | ~120 minutes

Suggested labels: Enhancement, Testing

Poem

A rabbit hops through paths and files,
With links that share and copies safe.
Variants wait behind a gate,
While execute bits find their place.
“Atomic paws!” the bunny sings,
And platform helpers sprout new wings.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR adds variant ingestion gating with a bumped schema version and atomic executable materialization, matching #314 and #316.
Out of Scope Changes check ✅ Passed The diff stays focused on platform-aware paths, manifests, and Unix workspace behavior.
Docstring Coverage ✅ Passed Docstring coverage is 87.10% which is sufficient. The required threshold is 50.00%.
Title check ✅ Passed The title follows conventional commit format and accurately summarizes the platform-aware paths, manifests, and Unix workspace changes.
Description check ✅ Passed The description is clearly related to the changeset and matches the PR objectives and file-level changes.
📋 Issue Planner

Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).

View plans for tickets: #314, #316

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/cross-platform-fixes

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@bobtista
bobtista marked this pull request as ready for review July 29, 2026 19:54
@coderabbitai coderabbitai Bot added Bug Something isn't working right Documentation Is documentation or complementary resource Enhancement New feature or request Testing Topic related to (unit) tests labels Jul 29, 2026
Comment thread GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs Outdated
Comment thread GenHub/GenHub.Core/Models/Manifest/ManifestVariantResolver.cs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 12

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@GenHub/GenHub.Core/Models/Manifest/ContentManifest.cs`:
- Around line 75-84: Ensure ContentManifest.Variants remains non-null when
deserialization supplies a JSON null value by coalescing null in its setter or
rejecting null during deserialization. Preserve the existing empty-list default
and add a regression test covering null Variants through ContentManifestPool and
ManifestIngestionGate.TryAccept.

In `@GenHub/GenHub.Core/Models/Manifest/ManifestVariantResolver.cs`:
- Around line 119-130: The declared entry-point resolution currently returns the
raw declaration instead of the matched manifest path. In
ManifestVariantResolver.cs lines 119-130, retain the matching ManifestFile from
the PathsMatch lookup and pass its RelativePath to
EntryPointResolution.Resolved; in ManifestVariantResolverTests.cs lines 142-156,
assert that normalized separator or case matches resolve to the file’s
RelativePath.
- Around line 115-118: Update the entry-point selection in the variant
resolution flow to avoid falling back to manifest.EntryPoint when Variants is
populated. Use only variant?.EntryPoint for matching variants, while preserving
the manifest entry point behavior for flat manifests without variants.
- Around line 133-160: Update the executable inference logic in the manifest
resolution flow to consider only files where both IsExecutable and
ExecutableFileClassifier.IsLegacyLaunchCandidate(RelativePath) are true. Use
this filtered set for the single-candidate, no-candidate, and ambiguity
outcomes, while leaving explicit EntryPoint resolution unchanged so scripts can
still be selected intentionally.

In
`@GenHub/GenHub.Linux/Infrastructure/DependencyInjection/LinuxServicesModule.cs`:
- Around line 4-12: In LinuxServicesModule.cs, alphabetize the shown using
directives according to the project’s existing convention and remove the
duplicate blank line around the service-registration section near line 46.
Preserve all imports and code behavior while clearing the StyleCop warnings.

In
`@GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/ExecutablePermissionIsolationTests.cs`:
- Around line 110-118: Replace the hand-rolled copy–chmod–move logic in the
affected tests with calls to the production
WorkspaceStrategyBase.EnsureExecutableAsync implementation through a test
strategy. Assert CAS isolation, executable permissions, destination replacement,
and cleanup of any temporary file, covering both referenced test sections
without duplicating the materialization algorithm.

In
`@GenHub/GenHub.Windows/Features/Workspace/WindowsSymlinkCapabilityProvider.cs`:
- Around line 27-34: Replace the Administrator membership check in
CanCreateSymlinks with a cached probe that attempts to create and remove a
temporary symbolic link, returning whether the operation succeeds. Ensure the
probe uses unique temporary paths, cleans up any created artifacts, and returns
false on failure while preserving the cached capability result for strategy
selection.

In `@GenHub/GenHub/Features/GameProfiles/Services/ProfileLauncherFacade.cs`:
- Around line 453-464: Ensure the symlink fallback represented by
effectiveStrategy is applied to the actual workspace configuration, not only
logged or used locally. Update the shared workspace-configuration construction
used by gameLauncher and PrepareWorkspaceAsync, or normalize the in-memory
profile before both paths, while preserving explicitly configured strategies and
inherited symlink defaults consistently.

In `@GenHub/GenHub/Features/Manifest/ManifestProvider.cs`:
- Around line 77-80: Centralize the ManifestIngestionGate validation currently
used in ManifestProvider and invoke it on every return path, including cached
manifests and both embedded-manifest flows. Ensure the installation overload
does not return an embedded variant when AddManifestAsync rejects it; rejected
manifests must throw ManifestValidationException with the game client ID and
rejection reason before reaching callers.

In `@GenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.cs`:
- Around line 691-714: Update the exception handling around executable
materialization in the relevant workspace preparation method to rethrow the
original exception after attempting temporary-file cleanup, including
cancellation exceptions. Remove the warning-only success path so failures from
copy, chmod, or move prevent the workspace from being recorded as usable; retain
best-effort cleanup and its debug logging.

In `@GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs`:
- Around line 66-76: Update CopyFromCasAsync to call CopyFileAsync with
destinationPath and casPath instead of CreateHardLinkAsync, ensuring the method
creates an independent copy of the resolved CAS content while preserving its
existing success and missing-path behavior.

In `@GenHub/GenHub/Features/Workspace/WorkspaceValidator.cs`:
- Around line 350-383: Update HasUnixExecutePermission to validate execute
access for the current process’s effective Unix identity rather than checking
any permission bit directly. Use an effective-access mechanism such as a
platform-specific faccessat wrapper with X_OK and AT_EACCESS, while preserving
the Windows behavior and returning false when the access check fails.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4c82cc30-68c1-49fb-8439-bf677a3a07f1

📥 Commits

Reviewing files that changed from the base of the PR and between 8e07339 and 64502bb.

📒 Files selected for processing (47)
  • GenHub/GenHub.Core/Constants/ManifestConstants.cs
  • GenHub/GenHub.Core/Interfaces/Workspace/ISymlinkCapabilityProvider.cs
  • GenHub/GenHub.Core/Models/Manifest/ArtifactVariant.cs
  • GenHub/GenHub.Core/Models/Manifest/ContentManifest.cs
  • GenHub/GenHub.Core/Models/Manifest/EntryPointResolution.cs
  • GenHub/GenHub.Core/Models/Manifest/ManifestIngestionGate.cs
  • GenHub/GenHub.Core/Models/Manifest/ManifestVariantResolver.cs
  • GenHub/GenHub.Core/Utilities/ExecutableFileClassifier.cs
  • GenHub/GenHub.Linux/Infrastructure/DependencyInjection/LinuxServicesModule.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Content/GitHubInferenceHelperTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameSettings/GamePathProviderTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ManifestDiscoveryServiceTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/ExecutablePermissionIsolationTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/FileOperationsServiceTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/GameProfileWorkspaceIntegrationTest.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/UnixFileOperationsServiceTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/ApplicationDataPathConventionTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Models/Manifest/ManifestIngestionGateTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Models/Manifest/ManifestVariantResolverTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Utilities/ExecutableFileClassifierTests.cs
  • GenHub/GenHub.Windows/Features/Workspace/WindowsSymlinkCapabilityProvider.cs
  • GenHub/GenHub.Windows/Infrastructure/DependencyInjection/WindowsServicesModule.cs
  • GenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostDeliverer.cs
  • GenHub/GenHub/Features/Content/Services/ContentDiscoverers/GitHubTopicsDiscoverer.cs
  • GenHub/GenHub/Features/Content/Services/Helpers/GitHubInferenceHelper.cs
  • GenHub/GenHub/Features/GameProfiles/Services/ProfileLauncherFacade.cs
  • GenHub/GenHub/Features/GameProfiles/ViewModels/FileTreeItem.cs
  • GenHub/GenHub/Features/GameSettings/GamePathProviderBase.cs
  • GenHub/GenHub/Features/GameSettings/GameSettingsService.cs
  • GenHub/GenHub/Features/GameSettings/LinuxGamePathProvider.cs
  • GenHub/GenHub/Features/GameSettings/MacOSGamePathProvider.cs
  • GenHub/GenHub/Features/GameSettings/WindowsGamePathProvider.cs
  • GenHub/GenHub/Features/Manifest/ContentManifestBuilder.cs
  • GenHub/GenHub/Features/Manifest/ContentManifestPool.cs
  • GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs
  • GenHub/GenHub/Features/Manifest/ManifestGenerationService.cs
  • GenHub/GenHub/Features/Manifest/ManifestProvider.cs
  • GenHub/GenHub/Features/Manifest/SteamManifestPatcher.cs
  • GenHub/GenHub/Features/Workspace/FileOperationsService.cs
  • GenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.cs
  • GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs
  • GenHub/GenHub/Features/Workspace/UnixNativeMethods.cs
  • GenHub/GenHub/Features/Workspace/UnixSymlinkCapabilityProvider.cs
  • GenHub/GenHub/Features/Workspace/WorkspaceValidator.cs
  • GenHub/GenHub/Infrastructure/DependencyInjection/ContentPipelineModule.cs

Comment thread GenHub/GenHub.Core/Models/Manifest/ContentManifest.cs Outdated
Comment thread GenHub/GenHub.Core/Models/Manifest/ManifestVariantResolver.cs
Comment thread GenHub/GenHub.Core/Models/Manifest/ManifestVariantResolver.cs
Comment thread GenHub/GenHub.Core/Models/Manifest/ManifestVariantResolver.cs Outdated
Comment thread GenHub/GenHub.Linux/Infrastructure/DependencyInjection/LinuxServicesModule.cs Outdated
Comment thread GenHub/GenHub/Features/Manifest/ManifestProvider.cs Outdated
Comment thread GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs
Comment thread GenHub/GenHub/Features/Workspace/WorkspaceValidator.cs Outdated
@kilo-code-bot

kilo-code-bot Bot commented Jul 29, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ContentManifestPoolTests.cs - Added 3 variant rejection tests
  • GenHub/GenHub/Features/GameProfiles/Services/ProfileLauncherFacade.cs - Updated log messages for symlink capability

Incremental Review: This review covers changes from commit 5b1a16051323a1959ac20f8b854f7f9f10c87c21 to f9e60e49fdc18bd22cdd411180205ca739a09ada.

Changes Analyzed:

  • Added test coverage for variant manifest rejection before content storage
  • Updated log and notification messages to attribute symlink strategy downgrades to capability rather than admin rights

Previous Issues: All 17 issues from the prior review remain unchanged (no files with those issues were modified in this incremental change).

Previous Review Summaries (3 snapshots, latest commit 5b1a160)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 5b1a160)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/WorkspaceValidatorTests.cs - Root user guard added
  • GenHub/GenHub/Features/Content/Services/ContentDiscoverers/GitHubTopicsDiscoverer.cs - Modern array syntax
  • GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs - Ingestion gate helper extracted

Previous review (commit eafdf81)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (14 changed files)
  • GenHub/GenHub.Core/Models/Manifest/ContentManifest.cs - Null-safe Variants handling
  • GenHub/GenHub.Core/Models/Manifest/ManifestVariantResolver.cs - Fixed entry point resolution
  • GenHub/GenHub.Linux/Infrastructure/DependencyInjection/LinuxServicesModule.cs - Import ordering
  • GenHub/GenHub.Windows/Features/Workspace/WindowsSymlinkCapabilityProvider.cs - Capability probe implementation
  • GenHub/GenHub.Windows/Infrastructure/DependencyInjection/WindowsServicesModule.cs - Import ordering
  • GenHub/GenHub/Features/GameProfiles/Services/ProfileLauncherFacade.cs - Strategy resolution fix
  • GenHub/GenHub/Features/Manifest/ManifestProvider.cs - Ingestion gating applied to all paths
  • GenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.cs - Exception propagation fix
  • GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs - Copy instead of hard link
  • GenHub/GenHub/Features/Workspace/UnixNativeMethods.cs - Effective-identity access check
  • GenHub/GenHub/Features/Workspace/WorkspaceValidator.cs - Proper execute permission check
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ContentManifestPoolTests.cs - Null variants test
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ManifestProviderTests.cs - Cached variant tests
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/ExecutablePermissionIsolationTests.cs - Production method testing
Review Notes

All incremental changes from commit 64502bb address the previously identified issues correctly:

  1. UnixFileOperationsService.cs - Now uses CopyFileAsync instead of CreateHardLinkAsync, creating independent copies
  2. ManifestVariantResolver.cs - Returns matchedFile.RelativePath instead of raw declaration, fixing case-sensitive launch issues
  3. WorkspaceStrategyBase.cs - Now throws exceptions after cleanup, preventing broken workspaces
  4. WindowsSymlinkCapabilityProvider.cs - Rewritten to use real capability probing instead of Administrator check
  5. ProfileLauncherFacade.cs - Added ResolveSupportedWorkspaceStrategy and applies effective strategy to in-memory profile
  6. ManifestProvider.cs - Added EnsureManifestAccepted and applies ingestion gating to all return paths
  7. WorkspaceValidator.cs - Uses UnixNativeMethods.CanExecute for proper effective-identity access checks

No new issues found. All changes are correct and improve the implementation.

Previous review (commit 64502bb)

Status: 0 New Issues | Recommendation: Address existing issues before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 0
Existing Issues (3)

This PR has 3 existing inline comments that should be addressed before merge:

  • GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs:74-75 - CAS copy creates hard links
  • GenHub/GenHub.Core/Models/Manifest/ManifestVariantResolver.cs:125-126 - Entry-point casing breaks Linux launch
  • GenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.cs:691-714 - Permission failures persist broken workspaces
Files Reviewed (47 files)
  • GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs
  • GenHub/GenHub/Features/Workspace/UnixNativeMethods.cs
  • GenHub/GenHub/Features/Workspace/UnixSymlinkCapabilityProvider.cs
  • GenHub/GenHub.Core/Models/Manifest/ManifestVariantResolver.cs
  • GenHub/GenHub.Core/Models/Manifest/ManifestIngestionGate.cs
  • GenHub/GenHub.Core/Utilities/ExecutableFileClassifier.cs
  • GenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.cs
  • GenHub/GenHub/Features/GameSettings/GameSettingsService.cs
  • GenHub/GenHub/Features/GameSettings/LinuxGamePathProvider.cs
  • GenHub/GenHub/Features/GameSettings/MacOSGamePathProvider.cs
  • GenHub/GenHub/Features/GameSettings/WindowsGamePathProvider.cs
  • GenHub/GenHub.Linux/Infrastructure/DependencyInjection/LinuxServicesModule.cs
  • GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs
  • GenHub/GenHub/Features/Workspace/WorkspaceValidator.cs
  • GenHub/GenHub/Features/Workspace/FileOperationsService.cs
  • And 32 other test and infrastructure files
Review Notes

This PR makes substantial improvements to GenHub's platform awareness across Windows and Unix. The changes include:

  • Platform-specific game path providers for Linux and macOS
  • Native Unix hard link support with proper error handling
  • Symlink capability detection
  • Deterministic manifest entry point and variant resolution
  • Executable permission isolation to prevent CAS mutations
  • Centralized executable file classification
  • Proper manifest variant gating

The architecture is sound and the cross-platform considerations are generally well-handled. No new issues were found beyond the 3 existing comments.


Reviewed by glm-4.7 · Input: 43.3K · Output: 4.3K · Cached: 365.7K

bobtista added 9 commits July 29, 2026 21:51
…then-move

Copy, set the mode on the copy, then rename over the destination. The previous copy/delete/move/chmod sequence exposed two states: destination missing, and destination present but not executable. The second is unrecoverable because verification never mutates, so the workspace stays broken for every later launch.

Closes #316.
… are migrated

Deliverers, validators, CAS reference counting and GC all still read ContentManifest.Files, which is empty for a variant manifest. Accepting one would deliver nothing while reporting success and record the wrong blobs in reference counting.

Adds a fail-closed gate applied in ManifestDiscoveryService and ManifestProvider, and a manifest format version identifying variant manifests so the rejection is attributable rather than presenting as a parse failure. Temporary: remove as part of the resolved-variant migration (#321).

Closes #314.
development builds warning-clean; these were introduced here. Covers using-directive ordering, comment spacing, and a platform guard the analyzer can follow for File.SetUnixFileMode inside the copy lambda.
@bobtista
bobtista force-pushed the feat/cross-platform-fixes branch from 64502bb to eafdf81 Compare July 29, 2026 20:56
@bobtista

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot removed Bug Something isn't working right Documentation Is documentation or complementary resource labels Jul 29, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs (1)

196-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated ingestion-gate check into a shared helper.

The same deserialize → ManifestIngestionGate.TryAccept → log-and-skip pattern is duplicated across LoadManifestAsync, DiscoverFileSystemManifestsAsync, and DiscoverEmbeddedManifestsAsync, differing only in the log message's context label and the post-acceptance action. Extracting a small helper (e.g., TryAcceptManifest(manifest, contextLabel, out rejectionReason) that performs the TryAccept call and logging) would remove the triplication while keeping the gate applied at all three ingestion points.

♻️ Proposed helper (illustrative)
+    private bool TryAcceptManifest(ContentManifest manifest, string context)
+    {
+        if (ManifestIngestionGate.TryAccept(manifest, out var rejectionReason))
+        {
+            return true;
+        }
+
+        logger.LogWarning("Skipping manifest at {Context}: {Reason}", context, rejectionReason);
+        return false;
+    }

Also applies to: 291-299, 329-337

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs` around lines 196
- 215, Extract the repeated ManifestIngestionGate.TryAccept and
rejection-warning logic from LoadManifestAsync,
DiscoverFileSystemManifestsAsync, and DiscoverEmbeddedManifestsAsync into a
shared helper such as TryAcceptManifest. Have the helper accept the manifest and
context label, perform the gate check, log rejection with the existing logger,
and return the acceptance result; update all three ingestion paths to call it
while preserving their distinct post-acceptance actions and context-specific
messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/WorkspaceValidatorTests.cs`:
- Around line 254-288: Update
ValidateWorkspaceAsync_OtherOnlyExecuteBit_ReturnsAccessWarning to skip when
UnixNativeMethods.GetEffectiveUserId() == 0, in addition to the existing Windows
guard. Keep the test’s current file-mode setup and assertions unchanged for
non-root Unix identities.

In `@GenHub/GenHub/Features/GameProfiles/Services/ProfileLauncherFacade.cs`:
- Around line 169-178: Update the fallback wording in
ProfileLauncherFacade.ResolveSupportedWorkspaceStrategy logging at
GenHub/GenHub/Features/GameProfiles/Services/ProfileLauncherFacade.cs:169-178
and the downstream log/notification at
GenHub/GenHub/Features/GameProfiles/Services/ProfileLauncherFacade.cs:452-463 to
state that symlinks are unavailable in the current environment. Remove
references implying missing elevation or administrator rights while preserving
the existing fallback behavior.

---

Outside diff comments:
In `@GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs`:
- Around line 196-215: Extract the repeated ManifestIngestionGate.TryAccept and
rejection-warning logic from LoadManifestAsync,
DiscoverFileSystemManifestsAsync, and DiscoverEmbeddedManifestsAsync into a
shared helper such as TryAcceptManifest. Have the helper accept the manifest and
context label, perform the gate check, log rejection with the existing logger,
and return the acceptance result; update all three ingestion paths to call it
while preserving their distinct post-acceptance actions and context-specific
messages.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c05fae93-aabb-497e-a43c-9043752317a5

📥 Commits

Reviewing files that changed from the base of the PR and between 64502bb and eafdf81.

📒 Files selected for processing (41)
  • GenHub/GenHub.Core/Constants/ManifestConstants.cs
  • GenHub/GenHub.Core/Interfaces/Workspace/ISymlinkCapabilityProvider.cs
  • GenHub/GenHub.Core/Models/Manifest/ArtifactVariant.cs
  • GenHub/GenHub.Core/Models/Manifest/ContentManifest.cs
  • GenHub/GenHub.Core/Models/Manifest/EntryPointResolution.cs
  • GenHub/GenHub.Core/Models/Manifest/ManifestIngestionGate.cs
  • GenHub/GenHub.Core/Models/Manifest/ManifestVariantResolver.cs
  • GenHub/GenHub.Linux/Infrastructure/DependencyInjection/LinuxServicesModule.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Content/GitHubInferenceHelperTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ContentManifestPoolTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ManifestProviderTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/ExecutablePermissionIsolationTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/FileOperationsServiceTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/GameProfileWorkspaceIntegrationTest.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/UnixFileOperationsServiceTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/WorkspaceStrategyBaseTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Workspace/WorkspaceValidatorTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/ApplicationDataPathConventionTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Models/Manifest/ManifestIngestionGateTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Models/Manifest/ManifestVariantResolverTests.cs
  • GenHub/GenHub.Tests/GenHub.Tests.Core/Utilities/ExecutableFileClassifierTests.cs
  • GenHub/GenHub.Windows/Features/Workspace/WindowsSymlinkCapabilityProvider.cs
  • GenHub/GenHub.Windows/Infrastructure/DependencyInjection/WindowsServicesModule.cs
  • GenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostDeliverer.cs
  • GenHub/GenHub/Features/Content/Services/ContentDiscoverers/GitHubTopicsDiscoverer.cs
  • GenHub/GenHub/Features/GameProfiles/Services/ProfileLauncherFacade.cs
  • GenHub/GenHub/Features/GameSettings/GameSettingsService.cs
  • GenHub/GenHub/Features/Manifest/ContentManifestBuilder.cs
  • GenHub/GenHub/Features/Manifest/ContentManifestPool.cs
  • GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs
  • GenHub/GenHub/Features/Manifest/ManifestGenerationService.cs
  • GenHub/GenHub/Features/Manifest/ManifestProvider.cs
  • GenHub/GenHub/Features/Workspace/FileOperationsService.cs
  • GenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.cs
  • GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs
  • GenHub/GenHub/Features/Workspace/UnixNativeMethods.cs
  • GenHub/GenHub/Features/Workspace/UnixSymlinkCapabilityProvider.cs
  • GenHub/GenHub/Features/Workspace/WorkspaceValidator.cs
  • GenHub/GenHub/Infrastructure/DependencyInjection/ContentPipelineModule.cs

…only test case

The deserialize, gate, log-and-skip sequence was repeated at all three ManifestDiscoveryService ingestion points, differing only in the context label. Collapsed into IsManifestAccepted.

The other-only execute-bit test now skips for uid 0. Root bypasses the permission bits, so faccessat reports execute access and the behaviour under test does not exist. geteuid is used rather than the user name, which is wrong under 'sudo -E' and for any uid-0 account named otherwise. GitHub's runners are non-root so this passed in CI, but it fails in a root container.
@bobtista
bobtista force-pushed the feat/cross-platform-fixes branch from eafdf81 to 5b1a160 Compare July 30, 2026 00:57
…than admin rights

The downgrade is driven by a real symlink-creation probe, so the messages no longer claim elevation is required. symlink(2) needs no privilege on Unix, and Windows grants it under Developer Mode, so telling the user to elevate sent them after a fix that would not help. The user-facing notification carried the same claim and is corrected too.

Genuine Windows UAC handling for Error 740 is unchanged.

Also covers the ingestion gate at the pool, which is the chokepoint every deliverer, resolver and detector reaches. The tests assert that a variant manifest is rejected before any content is stored and before any CAS reference is tracked; mis-tracked references are what corrupts reference counting and garbage collection, so returning a failure alone would not be enough. Both fail if the gate is removed.
Comment on lines 502 to +504
}

await EnsureExecutableAsync(file, targetPath, cancellationToken);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Executable isolation skips strategies

When a Unix profile uses FullCopy, HybridCopySymlink, or HardLink, those strategies materialize files through their own loops and bypass ProcessManifestFileAsync, so EnsureExecutableAsync never runs. Executables retain a non-executable mode in copy-based workspaces, while hard-linked executables are not detached from CAS as intended, causing launch failures or shared-inode permission changes.

Knowledge Base Used: Workspace Assembly

Prompt To Fix With AI
This is a comment left during a code review.
Path: GenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.cs
Line: 502-504

Comment:
**Executable isolation skips strategies**

When a Unix profile uses `FullCopy`, `HybridCopySymlink`, or `HardLink`, those strategies materialize files through their own loops and bypass `ProcessManifestFileAsync`, so `EnsureExecutableAsync` never runs. Executables retain a non-executable mode in copy-based workspaces, while hard-linked executables are not detached from CAS as intended, causing launch failures or shared-inode permission changes.

**Knowledge Base Used:** [Workspace Assembly](https://app.greptile.com/genhub/-/custom-context/knowledge-base/community-outpost/genhub/-/docs/workspace-assembly.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

await CreateHardLinkAsync(destinationPath, casPath, cancellationToken).ConfigureAwait(false);
return true;
}
catch (IOException ex) when (ex is not FileNotFoundException)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 EACCES bypasses copy fallback

When link(2) returns EACCES while HardLinkStrategy materializes a CAS file, CreateHardLinkAsync throws UnauthorizedAccessException, which is not caught by this IOException handler. The per-file copy fallback is skipped and workspace preparation aborts instead of producing an independent copy.

Knowledge Base Used: Workspace Assembly

Prompt To Fix With AI
This is a comment left during a code review.
Path: GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs
Line: 109

Comment:
**EACCES bypasses copy fallback**

When `link(2)` returns `EACCES` while `HardLinkStrategy` materializes a CAS file, `CreateHardLinkAsync` throws `UnauthorizedAccessException`, which is not caught by this `IOException` handler. The per-file copy fallback is skipped and workspace preparation aborts instead of producing an independent copy.

**Knowledge Base Used:** [Workspace Assembly](https://app.greptile.com/genhub/-/custom-context/knowledge-base/community-outpost/genhub/-/docs/workspace-assembly.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Comment on lines +107 to +108
return true;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security Workspace writes corrupt shared CAS

When a launched game or plugin writes to a non-executable content-addressable file in a hard-link workspace, the write changes the same user-writable inode stored under the CAS hash. Another profile can then materialize the modified bytes and receive corrupted or incorrect content. How this was verified: HardLinkStrategy requests real CAS hard links, while copy-on-write isolation applies only to files marked executable and ordinary materialization does not universally revalidate the blob hash.

Knowledge Base Used: Workspace Assembly

Prompt To Fix With AI
This is a comment left during a code review.
Path: GenHub/GenHub/Features/Workspace/UnixFileOperationsService.cs
Line: 107-108

Comment:
**Workspace writes corrupt shared CAS**

When a launched game or plugin writes to a non-executable content-addressable file in a hard-link workspace, the write changes the same user-writable inode stored under the CAS hash. Another profile can then materialize the modified bytes and receive corrupted or incorrect content. **How this was verified:** `HardLinkStrategy` requests real CAS hard links, while copy-on-write isolation applies only to files marked executable and ordinary materialization does not universally revalidate the blob hash.

**Knowledge Base Used:** [Workspace Assembly](https://app.greptile.com/genhub/-/custom-context/knowledge-base/community-outpost/genhub/-/docs/workspace-assembly.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@bobtista
bobtista merged commit a82537d into development Jul 30, 2026
7 checks passed
bobtista added a commit that referenced this pull request Jul 30, 2026
#328 made IGamePathProvider a required dependency and #329 added the macOS host, but neither could carry the macOS registrations: they reference types from the first and a module from the second, so they only compile once both are in. Each PR was green in isolation and the merged state is not — GenHub.app now aborts at container build with 'Unable to resolve service for type IGamePathProvider while attempting to activate GameSettingsService', cascading across roughly twenty descriptors including MainViewModel and IGameLauncher.

Registers IGamePathProvider, ISymlinkCapabilityProvider and IFileOperationsService for macOS. Only the first breaks startup; without the other two macOS silently copies instead of hard-linking and cannot reach symlink strategies, which are the behaviours #328 set out to fix.

Also moves the macOS test run before publish. GenHub.Tests.MacOS was already covered by the job's final 'Run Tests' step, but that sits after Publish and Smoke Test App Launch, so the startup crash killed the job first and the assertion naming the service never ran. MacOSHost_ResolvesEveryRequiredService already asserted this; it was reached too late to be useful. The project is skipped in the later sweep so it runs once.
bobtista added a commit that referenced this pull request Jul 30, 2026
…331)

#328 made IGamePathProvider a required dependency and #329 added the macOS host, but neither could carry the macOS registrations: they reference types from the first and a module from the second, so they only compile once both are in. Each PR was green in isolation and the merged state is not — GenHub.app now aborts at container build with 'Unable to resolve service for type IGamePathProvider while attempting to activate GameSettingsService', cascading across roughly twenty descriptors including MainViewModel and IGameLauncher.

Registers IGamePathProvider, ISymlinkCapabilityProvider and IFileOperationsService for macOS. Only the first breaks startup; without the other two macOS silently copies instead of hard-linking and cannot reach symlink strategies, which are the behaviours #328 set out to fix.

Also moves the macOS test run before publish. GenHub.Tests.MacOS was already covered by the job's final 'Run Tests' step, but that sits after Publish and Smoke Test App Launch, so the startup crash killed the job first and the assertion naming the service never ran. MacOSHost_ResolvesEveryRequiredService already asserted this; it was reached too late to be useful. The project is skipped in the later sweep so it runs once.
bobtista added a commit that referenced this pull request Jul 30, 2026
…port zero-exit stderr

Validation checked both games' declared roots regardless of which was launching, so a stale Zero Hour path could block a Generals launch that would have worked. It now checks only the launching game's root, taking the game type from the client.

A process exiting immediately with code 0 and no spawned child returned a bare message while the stderr drained moments earlier went unreported — the one diagnostic available for that case, and contrary to the point of the capture work in this branch.

Test changes: the native-client fixture discovery was duplicated across three classes and is now a shared NativeClientFixture, so they cannot diverge on when to skip; engine-only staging includes .so alongside .dylib, having been macOS-only; and SetUnixFileMode calls are guarded so the platform analyzer can follow them.

Also clears every warning in GenHub.Tests.Core, including five that reached development through #328 because I had only ever built the main project.
bobtista added a commit that referenced this pull request Aug 1, 2026
#332)

* feat(launching): surface native launch failures and set the loader search path

* test(launching): verify the real native client launches through GenHub

* test(launching): serialize real-client launches around the engine instance lock

* feat(launching): reach retail archives via install-path environment instead of copying them

* fix(launching): stop setting DYLD_LIBRARY_PATH and LD_LIBRARY_PATH

Measured unnecessary: the BGFX build declares every dependency as @executable_path/ with a matching rpath and launches with the variable cleared. Two costs outweighed a fallback for a build we do not ship: dyld consults DYLD_LIBRARY_PATH before @executable_path for leaf-name references, so a same-named library elsewhere silently wins; and the hardened runtime ignores it, so behaviour would change silently once GenHub is signed and notarized.

Closes #317.

* feat(launching): validate retail archive roots before spawning the engine

The engine reaches its main loop with zero content and no error when the archive roots are wrong: no exit code, no log line, nothing a host process can observe. Launch success therefore cannot distinguish a working game from an empty one, so the roots are checked before spawn and the launch fails with the offending root named.

Presence of at least one .big archive is the sentinel rather than a specific filename, which varies by localisation and version. An archive that exists but fails to mount still needs a machine-readable failure from the engine.

Closes #315.

* fix(launching): capture complete stderr when a launch fails

The buffer was read as soon as the process exited, but the asynchronous stderr handlers had not necessarily delivered their final lines — truncating diagnostics precisely on the path where they explain the failure. Wait via the parameterless WaitForExit overload, which also waits for redirected-output handlers, before reading.

The buffer now treats a null line as the end-of-stream signal rather than content, retains the head as well as the tail so startup context survives alongside the failure, and bounds line length and total size so a pathological writer cannot exhaust memory. Only stderr is redirected, so there is no stdout stream to drain.

Closes #318.

* chore: add stderr capture regression test and clear stack build warnings

Adds an end-to-end check that a process failing immediately still reports both the head and tail of its stderr. It does not deterministically pin the end-of-stream drain: removing the WaitForExit() call leaves it passing on macOS and .NET 8, because the handlers complete before the capture is read even at twenty thousand lines. The drain stands on the documented contract instead.

The GameLauncher fixture now uses a real retail root holding an archive. It previously pointed at a Windows path that does not exist off Windows, which the new pre-spawn validation correctly rejected.

Clears the warnings this stack introduced; development itself builds clean. Two remain, both predating these fixes and needing invasive reordering of long files: SA1204 in GameLauncher and SA1202 in WorkspaceStrategyBase.

* chore(workspace): order static helper before instance members

* chore(workspace): remove trailing class separator

* chore: clear native launch style warnings

* chore(workspace): keep member order valid after the executable materialization change

EnsureExecutableAsync became protected when the rethrow was added, which put it after a private helper. Moved it above, and dropped the trailing blank line the earlier reordering left.

* fix(launching): scope archive-root validation to non-Windows and normalize profile overrides

Two defects from review, both introduced by the earlier validation change.

Windows never reads CNC_ZH_INSTALLPATH or CNC_GENERALS_INSTALLPATH — it resolves install paths from the registry — so a Windows layout without loose top-level archives is not a misconfiguration. BuildEnvironmentVariables returns before setting them there, but validation reads the installation's declared paths, so it needed the same guard rather than inheriting it. Without it, valid Windows launches were rejected with a misleading message about missing archives.

A profile setting a root explicitly chooses the directory, not whether the trailing separator is applied. AddArchiveRoot returned early on an existing value and never normalized it, so the engine concatenated it with the archive filename to produce paths like '/path/toINIZH.big' and silently mounted nothing — precisely the failure this validation exists to prevent, reachable through the override path.

The variable names are an external contract with the engine, so they move to RetailArchiveConstants alongside the archive search pattern.

The normalization test fails against the previous early-return. The Windows guard asserts only on Windows and is a no-op elsewhere.

* test(launching): scope the archive-root rejection tests to non-Windows

Two of these asserted that validation rejects a bad root, which stopped being true on Windows once validation was guarded there — the platform guard was added without updating the tests that asserted the previous cross-platform behaviour, and Build Windows caught it.

Rejection is non-Windows behaviour by design; the Windows side is asserted separately by Validate_OnWindows_SkipsEntirely. The guards do not hollow the tests out: on non-Windows they still assert, and three still fail if the validation fix is reverted.

* fix(launching): scope archive validation to the launching game and report zero-exit stderr

Validation checked both games' declared roots regardless of which was launching, so a stale Zero Hour path could block a Generals launch that would have worked. It now checks only the launching game's root, taking the game type from the client.

A process exiting immediately with code 0 and no spawned child returned a bare message while the stderr drained moments earlier went unreported — the one diagnostic available for that case, and contrary to the point of the capture work in this branch.

Test changes: the native-client fixture discovery was duplicated across three classes and is now a shared NativeClientFixture, so they cannot diverge on when to skip; engine-only staging includes .so alongside .dylib, having been macOS-only; and SetUnixFileMode calls are guarded so the platform analyzer can follow them.

Also clears every warning in GenHub.Tests.Core, including five that reached development through #328 because I had only ever built the main project.

* fix(launching): validate the base Generals root when launching Zero Hour

Scoping validation to the launching game went one step too far. Zero Hour is an expansion and mounts the base Generals archives as well — this file's own remarks say so — so checking only the Zero Hour root let a stale or archive-free Generals root through and the game would run without base content: the same silent failure, one directory over.

Generals still reads only its own root, so launching it does not fail over a stale Zero Hour path. For Zero Hour the Generals root is checked only when declared, since an installation carrying the base archives itself needs no separate root; a declared root that is broken is an error.

The Zero Hour test fails if the second root is dropped.

* docs(launching): correct why an absent Generals root is tolerated and record the gap

The previous comment claimed a Zero Hour installation may carry the base archives itself. That was asserted, not verified, and is not the reason. The actual reason is that the engine mounts archives from the working directory as well, so base content may sit in the workspace rather than a retail root — the arrangement this mechanism replaces, but still valid.

Records the residual gap review identified: with no Generals root declared and no base content in the workspace either, Zero Hour starts with nothing to mount and this check cannot tell. Archive filenames are arbitrary, so testing for '*.big' proves nothing about base content; a workspace check was considered and rejected because a Zero Hour workspace always contains archives and it would always pass. Closing it needs the engine to report a failed mount.

* docs(launching): correct the engine-behaviour claim behind archive-root validation

The comments asserted the engine reaches its main loop with zero content and no error, and that no post-spawn check could tell. Verified against bobtista/GeneralsGameCode at bobtista/topic/trunk, that is false for the cases described: a root holding no archives aborts during initialisation with exit 1 and a ReleaseCrashInfo.txt, and an unmountable archive also writes '[ggc] ARCHIVE MOUNT FAILED' to stderr. Neither reaches the main loop.

The claim came from the plan and was repeated here without ever being checked against the engine, which is public.

Validating before spawn still earns its place, for reasons that are actually true: exit 1 is generic, the stderr sentinel exists only in StdBIGFileSystem and not the Win32 twin, and ReleaseCrash shows a system-modal dialog before _exit(1) so a host-launched child on Windows hangs instead of dying. The comments now say that, and bound what the .big sentinel can detect.

* test(launching): validate the fixture override and recognise versioned Linux libraries

Three review findings, all in the integration fixtures.

The GENHUB_NATIVE_CLIENT_DIR override only checked the directory existed, while the discovered default also required the engine binary. A directory without one is a misconfigured override and these tests should skip, not fail; both paths now validate the same way.

The '.so' suffix test missed versioned libraries such as libSDL3.so.0, which is the common shape of a shipped Linux library, and a second copy of the same filter in the manifest fixture had not been updated at all when '.so' was added earlier. Both now use a shared NativeClientFixture.IsDynamicLibrary.

The manifest fixture asserted only that something executable was present, so it would have passed with just the binary and never exercised library handling. It now requires a captured dynamic library.

The predicate is unit-tested because every test that uses it skips without a local engine install, so it would otherwise never run in CI. Writing those tests caught a real bug: matching on '.so' found the coincidental one inside '.sound' and stopped, so 'mylib.sound.so.1' was rejected. It matches on '.so.' now, and that case is pinned.

* fix(launching): match retail archives case-insensitively when validating roots

* fix(launching): ground archive-root failure messages in the verified engine abort

* chore(launching): reference launch constants in tests and fix stale name references
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement New feature or request Testing Topic related to (unit) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant